-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make REMAPPING_USE_OM4_SUBCELLS the default #758
base: dev/gfdl
Are you sure you want to change the base?
Conversation
In addition to REMAPPING_USE_OM4_SUBCELLS, for ALE remapping, there are several parameters of the form XXX_REMAPPING_USE_OM4_SUBCELLS, where XXX identifies the target, and they all currently default to True. To simplify setting them all to False, which is recommended, the defaults for the XXX versions is changed to the value of REMAPPING_USE_OM4_SUBCELLS. Answers are only changed if REMAPPING_USE_OM4_SUBCELLS is set to False and the default (now False) is used for one or more of the other parameters. In such cases the original behaviour can be recovered by explicitly setting the other parameters to True.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/gfdl #758 +/- ##
=============================================
- Coverage 36.67% 16.06% -20.62%
=============================================
Files 278 118 -160
Lines 84269 30498 -53771
Branches 15869 5602 -10267
=============================================
- Hits 30908 4898 -26010
+ Misses 47532 25147 -22385
+ Partials 5829 453 -5376 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These systematic changes to the defaults of subsidiary remapping parameters to follow an overall parameter is a very good idea, and I think that these changes will eventually be accepted exactly as they are written. However, this PR should be included in the set of changes with the previously agreed upon default changes that we will be putting in as a separate targeted PR to main, and it should only be merged into dev/gfdl when we are ready to handle those default changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - defaulting these parameters to a main parameter will be a lot less work for everyone.
In addition to REMAPPING_USE_OM4_SUBCELLS, for ALE remapping, there are several parameters of the form XXX_REMAPPING_USE_OM4_SUBCELLS, where XXX identifies the target, and they all currently default to True.
To simplify setting them all to False, which is recommended, the defaults for the XXX versions is changed to the value of REMAPPING_USE_OM4_SUBCELLS.
Answers are only changed if REMAPPING_USE_OM4_SUBCELLS is set to False and the default (now False) is used for one or more of the other parameters. In such cases the original behaviour can be recovered by explicitly setting the other parameters to True.